fix(security): walk the full AST in Scanner.Scan for ScanSQL parity - #879

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
gosqlx-website	Ready	Preview	Oct 28, 2026 7:30pm UTC

Claude finished @ajitpratap0's task in 23s —— View job

Review of PR #879: walk the full AST in Scanner.Scan

I read the diff and checked ast.Walk and ast.Inspect in pkg/sql/ast/visitor.go. I did not run the test suite, because the shell sandbox blocked go test here. I'm relying on the PR's reported task check and test:cbinding results.

Overall: The change looks sound. It replaces about 200 lines of hand-written traversal, which silently skipped many node types, with one ast.Inspect pass. That removes a whole class of "forgot to descend into X" bugs.

What I verified

• Ancestor stack stays balanced. Walk calls Visit(nil) only after it has descended into a node's children. When the callback returns false, as in the pruned text-bodied nodes and the JoinClause.Left skip, it never calls Visit(nil). Those paths also return before the append, so each push has exactly one pop.
• Concurrency. ancestors is local to each Scan call, so scans share no state. This matches the concurrent-scan test.
• No double reporting. FunctionCall is checked once by the walk. scanTableReference now only checks the table-function name, and the walk visits the TableFunc call itself separately. Text-bodied nodes are pruned, so the text and AST paths don't overlap.
• Tautology fix. Requiring leftIdent.Table == rightIdent.Table correctly stops a.id = b.id from being flagged.

Suggestions (non-blocking)

1. Tautology check on Filter. s.scanCondition(n.Filter, result) runs inside the FunctionCall case. That is correct, but it is the only condition scan done outside a statement-level case. A short comment saying so would help. Also confirm that Children() for FunctionCall includes Filter, so functions inside it get visited (the PR's FILTER test likely covers this).
2. Stray blank line. In scanFunctionCall, removing the argument recursion leaves an empty line before the closing } at about line 1153 of scanner.go. gofmt accepts it, but it should be removed.
3. Parent check on SetOperation. The parent.(*ast.SetOperation) check relies on Left and Right being direct children. If a parenthesized wrapper node is ever introduced between a set operation and its operand, the nested operation would get a second UNION check and report duplicates. A test with a parenthesized operand (a UNION (b UNION c)) would pin this down. The PR mentions nested UNION cases, so this may already be covered.
4. Allocation cost. The benchmark growth (about 25–200 ns up to about 160–1250 ns, 7–16 allocs) is acknowledged and reasonable. If it ever matters, the main cost is Children() slice allocation inside Walk, which is outside this PR's scope. A Scan benchmark on a large query (many CTEs or joins) would help catch regressions.
5. Behavior change. Scan now returns more findings for nested patterns and fewer tautology findings for qualified equalities. Both are documented in the PR description and CHANGELOG, which is good. Downstream consumers (cbinding, LSP, CLI) that count findings may see different numbers, so mention this in the release notes.

Tests

ast_coverage_test.go is thorough: 50 positive cases across six dialects, a Scan vs ScanSQL parity check, negatives, and a concurrency test. One gap: add a negative case that a.id = a.id still IS flagged, to lock in the qualified-identifier edge of the tautology fix.

No blocking issues found.

Claude finished @ajitpratap0's task in 19s —— View job

Review of PR #879: walk the full AST in Scanner.Scan

I read the scanner.go diff against origin/main. I did not run the tests. I'm relying on the PR's reported task check and test:cbinding results.

Overall: The change looks good. Replacing the hand-written traversal with a single ast.Inspect pass removes the "forgot to descend into X" class of gaps. It also makes Scan and ScanSQL agree.

Checked

• Ancestor stack. Every path that returns false does so before the append, so no Visit(nil) pop is owed for it. Every pushed node gets exactly one pop. This holds for the pruned text-bodied nodes and for the JoinClause.Left skip.
• Concurrency. ancestors is local to each call, so scans share no state.
• No double reporting. scanFunctionCall now checks only the call's own name, and the walk visits each FunctionCall once. scanTableReference checks only the table-function name. Text-bodied nodes are pruned, so the text and AST paths don't overlap.
• Qualified tautologies. Adding leftIdent.Table == rightIdent.Table correctly stops a.id = b.id from being flagged.

Suggestions (non-blocking)

1. Stray blank line. scanFunctionCall (pkg/sql/security/scanner.go, near the end of the function) now has an empty line before the closing }. This is left over from removing the argument recursion. Please delete it.
2. Comment on the FunctionCall case. s.scanCondition(n.Filter, result) is the only condition scan that doesn't sit in a statement-level case. A one-line comment would help. Please also confirm that FunctionCall.Children() includes Filter, so functions inside the FILTER clause get visited. The FILTER test probably covers this.
3. Parent-based SetOperation skip. The parent.(*ast.SetOperation) check relies on operands being direct children. Please confirm that the test file includes a UNION (b UNION c) and (a UNION b) UNION c. It should also include a set operation inside a CTE or subquery that sits under another set operation, to pin down the no-duplicate behaviour. The removed code's comment shows these parenthesized forms were a known case.
4. Test gap. Add a case that a.id = a.id is still flagged as a tautology. Add another that a.id = b.id is not. The second locks in the qualifier fix. The PR describes it but I didn't see it listed.
5. Performance. The cost rise (about 25–200 ns to about 160–1250 ns, 7–16 allocs) is mostly Children() slice allocation inside Walk. That is reasonable for complete coverage and outside this PR's scope. A Scan benchmark on a large query, such as many CTEs or joins, would help catch regressions later.
6. Release notes. Downstream consumers that count findings (CLI, LSP, cbinding) will see different numbers. More findings appear for nested patterns, and fewer appear for qualified equalities. The CHANGELOG entry covers this. Please also mention it in the release notes.

No blocking issues found.